-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for lttng-ust log4j2 #25
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Kienan Stewart <[email protected]>
bf8a038
to
4749a52
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this.
Adding a few notes to improve the patch.
vlttng/venv.py
Outdated
'log4j-core-{}.jar'.format(log4j2_version), | ||
'log4j-1.2-api-{}.jar'.format(log4j2_version), | ||
] | ||
_pinfo('Download Apache {}'.format(log4j2_name)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just Download Apache Log4j 2
is fine for this info.
vlttng/venv.py
Outdated
log4j2_version, log4j2_zipfile), log4j2_zipfile) | ||
self._runner.mkdir_p(log4j2_name) | ||
self._runner.unzip(log4j2_zipfile, log4j2_name) | ||
for src_jar, dest_jar in zip(log4j2_jars, self._paths.log4j2_jars): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NL above.
Also this zip looks very flimsy. You change the order in log4j2_jars
or self._paths.log4j2_jars
and this doesn't work anymore. Can you think about some way to infer one from the other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
vlttng/venv.py
Outdated
if '--enable-java-agent-all' in project.configure or \ | ||
'--enable-java-agent-log4j2' in project.configure: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same line is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
vlttng/venv.py
Outdated
@@ -440,7 +453,9 @@ def _create_project_instructions_lttng_tools(self, project): | |||
def _create_project_instructions_lttng_ust(self, project): | |||
instructions = self._create_project_instructions_generic_autotools(project) | |||
instructions.add_env = { | |||
'CLASSPATH': self._paths.log4j_jar, | |||
'CLASSPATH': "{}".format( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Useless format()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
vlttng/venv.py
Outdated
@@ -440,7 +453,9 @@ def _create_project_instructions_lttng_tools(self, project): | |||
def _create_project_instructions_lttng_ust(self, project): | |||
instructions = self._create_project_instructions_generic_autotools(project) | |||
instructions.add_env = { | |||
'CLASSPATH': self._paths.log4j_jar, | |||
'CLASSPATH': "{}".format( | |||
":".join([self._paths.log4j_jar] + self._paths.log4j2_jars) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Single quotes.
This whole thing may be just after CLASSPATH:
on the same line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
vlttng/venv.py
Outdated
return [ | ||
os.path.join(self.share_java, 'log4j2-api.jar'), | ||
os.path.join(self.share_java, 'log4j2-core.jar'), | ||
os.path.join(self.share_java, 'log4j2-1.2-api.jar'), | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List of names.
Then
return [os.path.join(self.share_java, f) for f in file_names]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
vlttng/venv.py
Outdated
self._runner.wget('https://dlcdn.apache.org/logging/log4j/{}/{}'.format( | ||
log4j2_version, log4j2_zipfile), log4j2_zipfile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Align arguments:
self._runner.wget('https://dlcdn.apache.org/logging/log4j/{}/{}'.format(log4j2_version,
log4j2_zipfile),
log4j2_zipfile)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
012385f
to
eac40b9
Compare
Signed-off-by: Kienan Stewart <[email protected]>
eac40b9
to
cc1fa23
Compare
No description provided.